Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AP_Camera: Add mount angles in camera log message #23837

Merged
merged 12 commits into from
Jul 26, 2023

Conversation

khanasif786
Copy link
Contributor

@khanasif786 khanasif786 commented May 19, 2023

This PR adds logging of the mount's desired and actual roll, pitch and yaw angles. Both body-frame and earth-frame yaw angles are logged but only one of either DesYawE and DesYawB will ever be non-NaN (because the target will only ever be one of these two).

The mount's angles are logged at 10hz and then again whenever a camera picture is taken (in which case the CAM and MNT message's TimeUS fields will be identical).

Also included are numerous fixes found during testing:

  • ViewPro driver pitch angle report ing fix (affected both cpp and lua drivers)
  • DJI RS2 driver's angle reporting fix. The target roll-angle was also reversed (due to this MP bug)
  • Gremsy fix to attitude reporting
  • CAMx_TYPE gets the "None" value

fixes #23816 which is an issue from #20985

This has been tested on real hardware including:

  • SIYI A8 and ZR10
  • Gremsy PixyU
  • DJI RS2
  • ViewPro A10TR
  • Xacti GB100

Below is a sample picture of the tests run on the DJI RS2. We can see how the actual angles follow behind the desired angles.
image

@khanasif786 khanasif786 marked this pull request as draft May 19, 2023 18:35
@khanasif786 khanasif786 force-pushed the cam_msg branch 2 times, most recently from 7d23eef to 657f358 Compare May 19, 2023 19:08
Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any discussion on the relative merits of this data.

Isn't it redundant with the existing mount logging?

If we log too much data then people start getting gaps in their logs - and that often makes the logs useless. So we shouldn't log redundant data when we don't need to.

libraries/AP_Camera/AP_Camera_Logging.cpp Outdated Show resolved Hide resolved
libraries/AP_Camera/LogStructure.h Outdated Show resolved Hide resolved
@rmackay9
Copy link
Contributor

rmackay9 commented May 19, 2023

@peterbarker,

I think it's important that the CAM message include all the data that someone needs to figure out the location and attitude of the camera at the moment the picture was taken. People are very sensitive to timing so I think it needs to be logged at exactly the same time. I also think that users will struggle with the external tools if that requires pulling data from multiple messages.

BTW, this CAM message is output only when pictures are taken so it's not going to take up any space in the overall scheme of things.

One question I have is whether we should also log the mount's yaw in earth-frame. I think the new mount_yaw field is body-frame meaning that users will need to add it to the vehicle's yaw. If that's too annoying we could perhaps have both MYawBF and MYawEF fields.

FYI @CraigElder

@khanasif786 khanasif786 force-pushed the cam_msg branch 2 times, most recently from 03d061f to b4d53ce Compare May 20, 2023 09:22
@khanasif786 khanasif786 marked this pull request as ready for review May 20, 2023 10:31
@peterbarker
Copy link
Contributor

I retract my objections here - camera messages are low-bandwidth and having the mount data directly against it makes some sense.

Does need to pass CI, 'though :-)

libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
libraries/AP_Mount/AP_Mount_Backend.cpp Outdated Show resolved Hide resolved
@rmackay9
Copy link
Contributor

Thanks for this, in general it looks pretty good.

I really like that you've tried to add both the target angle and the actual angles but I see this is making the change quite a bit more difficult because the target angle may not be readily available from all backends. If this is the case you could reduce the complexity and just log the current actual angles.

@khanasif786 khanasif786 force-pushed the cam_msg branch 2 times, most recently from d08a467 to 4933326 Compare June 18, 2023 20:27
@khanasif786
Copy link
Contributor Author

Pardon me for stylistic mistakes in this one, Actually i wanna know if the approach is right. It stores the target angles in mnt_target for all the possible mount modes. However it costs a lot more flash space.

@rmackay9
Copy link
Contributor

I think it is logging at 50hz but really 10hz is sufficient.
image

@rmackay9
Copy link
Contributor

There seems to be an error in the DesYawE calculation. I pointed a drone south and clicked on the MP map and selected, "Point Camera Here" to a Location that was South-East of the vehicle. The Desired Yaw E should have been about 140-ish. The YawE seems correct.
image

@khanasif786 khanasif786 force-pushed the cam_msg branch 2 times, most recently from b992e80 to 46356a2 Compare July 19, 2023 07:08
@rmackay9 rmackay9 dismissed peterbarker’s stale review July 20, 2023 00:48

I think Peter's request has been superceded by other changes.

Copy link
Contributor

@rmackay9 rmackay9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready to go but I'd appreciate any other feedback from other devs because it is a fairly extensive change.

@tridge tridge merged commit 7de2dac into ArduPilot:master Jul 26, 2023
82 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AP_Camera: log corresponding gimbal mount angles in CAM messages
5 participants